-
Notifications
You must be signed in to change notification settings - Fork 103
Implement enter end exit animation for Dialog #2374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I'm not sure this is a good idea, in several aspects:
|
|
Customization is key! There should be a default value trying to mimic platform behaviour (per Os and platform like IOS, MacOs, Windows, etc) but it should still be customizable on dev's side |
|
@m-sasha , thanks for the feedback. If you have any ideas how to organize the API, please let me know.
As animation is a behaviour defined by the platform itself, it seems reasonable to define the basic animation in the 'ui' module. However, there are caveats, which I will explain further in the text.
It's good that you raised this question. To answer it, we must consider that a Popup is not equivalent to an Alert Dialog (or UIAlertController on iOS) — it can be used for a much wider range of use-cases. For this reason, I don't think we need to copy all the Alert Dialog animation variations for all the platforms and their various versions. It's much better to add a common wrapper around the system controls to have the native alerts than to mimic their behaviour.
On Android they are using Compose code to draw the Dialog's content and platform-provided scrim/appearance animations.
I don't have a strong opinion on that; it's a question for the designers. What I am sure about is that we should not blindly copy the alert animation as the dialog animation.
Until we have design opinion, I'd go with the one used on Android. |
|
@ApoloApps , please read my comment to @m-sasha . Dialogs can also be used for UI like this. I don't think they should have the same animation as alerts. |
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
…appearance # Conflicts: # compose/ui/ui/api/desktop/ui.api # compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ImageComposeSceneTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
m-sasha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a complete review, but we talked offline about a possible refactoring before continuing with the review.
compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ImageComposeSceneTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/ImageComposeSceneTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/TestUtils.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Dialog.skiko.kt
Outdated
Show resolved
Hide resolved
m-sasha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the name of showDialog, looks good to me.
This reverts commit 8903902. (#2374) Fixes: https://youtrack.jetbrains.com/issue/CMP-9179 ## Release Notes N/A
Animation
iOS:
Simulator.Screen.Recording.-.iPhone.16.-.2025-10-23.at.16.01.19.mp4
Desktop:
Screen.Recording.2025-10-23.at.15.50.47.mov
Comparing with Android:
Screen.Recording.2025-09-01.at.16.03.32.mov
Fixes https://youtrack.jetbrains.com/issue/CMP-4365/iOS-iOS-Dialog-has-no-enter-and-exit-animation
(The feature was reverted due to https://youtrack.jetbrains.com/issue/CMP-9179/Desktop-Web-Crash-when-closing-ModalWideNavigationRail)
Release Notes
N/A